Skip to content

Added method DateTime::createFromImmutable() [follow-up for #1145] #2484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

Majkl578
Copy link
Contributor

This basically resurrects PR #1145 which was initially merged, but then reverted because of some not really relevant objections.

The primary purpose of this method should be interoperability between DateTime and DateTimeImmutable. The further already has createFromMutable method, but the former has none. So if one wants to convert mutable to immutable, it's as easy as:

DateTime::createFromImmutable($dateTime)

but to convert immutable to mutable, it requires annoying boilerplate:

DateTime::createFromFormat(\DateTime::ISO8601, $dateTime->format(\DateTime::ISO8601), $dateTime->getTimezone())

There are still tons of libraries that rely only on DateTime class (looking at you, Doctrine). Of course, any new code should use the immutable variant, but it's not always feasible as such libraries wouldn't understand the immutable type - therefore it requires conversion.

I still believe it was a wrong decision to revert this before PHP 7, especially because the only arguments against were totally off topic / invalid.
Quoting @derickr's response from the mailing list, which was the only objection as far as I know:

When the factory method was added, we had this same discussion. And the
discussions lead to the conclusion that it does not make sense to have
this "createFromImmutable" factory method on DateTime. Basically with
the same reasons that people should type hint against the Interface
instead. To emulate that in PHP 5.5 and lower, users can just do:

class DateTimeInterface extends DateTime {}

I know, that's a bit of a hack.

These arguments are totally irrelevant. This is not a factory method, it's a method for interoperability and compatibility. Pretty much like it'd be $mutable->toImmutable() or $immutable->toMutable().
Also the hack is totally meaningless and kind of funny, since the libraries that use DateTime are obviously doing it for purpose (i.e. backwards compatibility - semver anyone?).

I'd really like to see this resolved befrore PHP 7.2. It has already been blocked for two years and @derickr didn't even bother to reply to objections on the original PR...

PHP_ME(DateTime, __set_state, NULL, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
PHP_ME(DateTime, __construct, arginfo_date_create, ZEND_ACC_CTOR|ZEND_ACC_PUBLIC)
PHP_ME(DateTime, __wakeup, NULL, ZEND_ACC_PUBLIC)
PHP_ME(DateTime, __set_state, NULL, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change white space with functional changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just following the alignment, but ok, reverted.

@@ -27,7 +27,7 @@ object(ReflectionClass)#%d (1) {
string(8) "DateTime"
}
..and get names of all its methods
array(18) {
array(19) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of this test? (I realise, it has nothing to do with the PR). I'd say we remove it as it just tests that Reflection works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was asking the same question myself, but just cowardly changed the expectf, because I didn't want to remove tests in the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is extremely important, since reflection of internal classes is a constant SNAFU

}
if (old_obj->time->tz_info) {
new_obj->time->tz_info = old_obj->time->tz_info;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines (2842-2849) should be replaced by using timelib_time_clone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, also changed createFromMutable to do so (which I simply duplicated).

@Majkl578 Majkl578 force-pushed the datetime-createfromimmutable branch from 02970e8 to 810d40a Compare April 17, 2017 17:50
@Majkl578
Copy link
Contributor Author

@derickr Thanks for your review, adressed your comments. What do you think about adding this method now, two years later? I still think it'd be a useful addition for interoperability between mutable & immutable datetimes. ☺️

@Majkl578
Copy link
Contributor Author

Travis failure seems unrelated:

The job exceeded the maximum time limit for jobs, and has been terminated.

@hikari-no-yume
Copy link
Contributor

hikari-no-yume commented Apr 17, 2017

but to convert immutable to mutable, it requires annoying boilerplate:

DateTime::createFromFormat(\DateTime::ISO8601, $dateTime->format(\DateTime::ISO8601), $dateTime->getTimezone())

I think that boilerplate is, in itself, a good reason to add this. It's an unfortunately not-uncommon thing to need to do, and I imagine it would be easy to mess up (losing or mangling data) if you do it yourself, as well as being plain inefficient.

@enumag
Copy link

enumag commented Apr 17, 2017

@Majkl578 In my opinion it has very limited use unless it is createFromInterface implemented on both DateTime and DateTimeImmutable.

@Majkl578
Copy link
Contributor Author

Majkl578 commented Apr 17, 2017

@enumag From the point of the interface, it doesn't make much sense. Since the interface doesn't know its implementors (OO principle), it could only accept itself and return itself.
So the signature would look as such (simplified code):

ínterface DateTimeInterface {
    static function createFromInterface(DateTimeInterface $other) : DateTimeInterface;
}

Therefore it would be in no way different from __clone (just static). Besides, it even looks strange.

Also, since the implementors can't introduce more specific return types (return types are invariant), they could only return the interface as well (doesn't help static analysis).
Similar for parameter types, implementors can't introduce more specific parameter types (which would violate LSP), they would be forced to accept itself as well (doesn't help static analysis either).
Because of the points above, such method would serve two purposes: a) clone/duplicate itself (or throw exception, which would be strange) and b) reinterpret the sibling.
I'm generally -1 on such overly generic method.

@Majkl578 Majkl578 force-pushed the datetime-createfromimmutable branch from 810d40a to 96476ce Compare April 17, 2017 23:48
Copy link
Member

@derickr derickr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Majkl578:

@derickr Thanks for your review, adressed your comments. What do you think about adding this method now, two years later? I still think it'd be a useful addition for interoperability between mutable & immutable datetimes. ☺️

I don't care enough about this method to be here or not.

But I did add some more comments to the code.

NEWS Outdated
@@ -51,6 +51,8 @@ PHP NEWS
. Fixed bug #74404 (Wrong reflection on DateTimeZone::getTransitions).
(krakjoe)
. Fixed bug #74080 (add constant for RFC7231 format datetime). (duncan3dc)
. Added DateTime::createFromImmutable() method.
(bugs dot php dot net at majkl578 dot cz, Rican7)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be an issue for this, and the issue should be mentioned in the NEWS file, just like the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -2816,7 +2822,28 @@ PHP_METHOD(DateTimeImmutable, __construct)
}
/* }}} */

/* {{{ proto DateTimeImmutable::createFromMutable(DateTimeZone object)
/* {{{ proto DateTime::createFromImmutable(DateTimeImmutable object)
Creates new DateTime object from an existing immutable DateTimeImmutable object.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not make more sense to group this with the other DateTime methods, instead of just before the DateTimeImmutable/createFromMutable method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where exactly should I put it then? There are __construct for mutable & immutable together, __set_state for both together as well...

@@ -27,7 +27,7 @@ object(ReflectionClass)#%d (1) {
string(8) "DateTime"
}
..and get names of all its methods
array(18) {
array(19) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@enumag
Copy link

enumag commented Apr 18, 2017

@Majkl578 I didn't mean adding the method to the ineterface. I meant this:

class DateTime implements DateTimeInterface {
    static function createFromInterface(DateTimeInterface $other) : DateTime {...}
}
class DateTimeImmutable implements DateTimeInterface {
    static function createFromInterface(DateTimeInterface $other) : DateTimeImmutable {...}
}

@drAlberT
Copy link

hope we will see the light .. ;(

@DennisBirkholz
Copy link
Contributor

I really don't like this (as I don't like the createFromMutable() method). Consider the following example:

<?php
    function doDomainSpecificTransformationWithDate(\DateTimeInterface $originalDate) {
        if ($originalDate instanceof \DateTimeImmutable) {
            $changeableDate = \DateTime::createFromImmutable($originalDate);
        } else {
            $changeableDate = clone $originalDate;
        }
        ...
    }

So the \DateTimeInterface typehint does not help you at all as you always have to distinguish between all available implementations. I prefer the solution suggested by @enumag even though I don't really like the method name. Maybe createFromObject or createFromDate or createFromOther?

@Majkl578 Majkl578 force-pushed the datetime-createfromimmutable branch from 96476ce to 495b260 Compare May 28, 2017 21:02
@Majkl578
Copy link
Contributor Author

Majkl578 commented May 28, 2017

I didn't mean adding the method to the interface.

If you don't, the call wouldn't be type safe (any static analyser like PHPStan would fail otherwise due to undefined method) and you would end up with instanceof check either DateTime or DateTimeImmutable anyway.

So the \DateTimeInterface typehint does not help you at all as you always have to distinguish between all available implementations.

This is correct. Most of the time when you need this sort of methods is a compatibility between legacy and modern code, where you'd be using a concrete type anyway. This is also why the argument and return type are not DateTimeInterface.

As I've already said, it doesn't make sense to make a generic method for both DateTime and DateTiumeImmutable in the interface. And even if we did, the method would work in a half-redundant way - it would either transform the other type or duplicate __clone.

@nikic
Copy link
Member

nikic commented Jun 23, 2017

@derickr Is this okay to merge?

@derickr
Copy link
Member

derickr commented Jul 3, 2017

@nikic I don't particularly have an opinion, but I think it would be good if somebody would look at this (@Ocramius perhaps?) to validate the ideas about the typehint continuing to work, and this not going to cause any other API/compat issues.

@@ -27,7 +27,7 @@ object(ReflectionClass)#%d (1) {
string(8) "DateTime"
}
..and get names of all its methods
array(18) {
array(19) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is extremely important, since reflection of internal classes is a constant SNAFU

@@ -53,6 +53,7 @@ PHP_FUNCTION(getdate);
PHP_METHOD(DateTime, __construct);
PHP_METHOD(DateTime, __wakeup);
PHP_METHOD(DateTime, __set_state);
PHP_METHOD(DateTime, createFromImmutable);
PHP_FUNCTION(date_create);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say fromImmutable, since you aren't really creating it, but rather casting it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but there is already DateTimeImmutable::createFromMutable(), so the naming is chosen mostly for consistency. :/

@Majkl578
Copy link
Contributor Author

ping for 7.2

@Ocramius
Copy link
Contributor

@Majkl578 the window for 7.2 is already closed, so this can sadly only land in 7.3

@Majkl578
Copy link
Contributor Author

7.2 isn't in RC phase afaik, is it? Should be fine then.
Anyway, 4 months for such simple addition... :(

@nikic
Copy link
Member

nikic commented Aug 25, 2017

Merged as d9d13ab into master.

Feature freeze is at beta, not RC. However, this kind of addition can still go in under the "small self-contained feature addition" clause, if the RMs approve. @remicollet @sgolemon What do you think about landing this in 7.2?

@nikic nikic closed this Aug 25, 2017
@Majkl578 Majkl578 deleted the datetime-createfromimmutable branch September 8, 2017 01:15
@Majkl578
Copy link
Contributor Author

Majkl578 commented Sep 8, 2017

@nikic: Thanks! 🎉

Last try before giving up - @remicollet @sgolemon would this be eligible for freeze exception as a small self-contained addition? Thanks.

@Rican7
Copy link
Contributor

Rican7 commented Jan 25, 2018

Oh wow. I'm just now seeing this.

I want to thank @Majkl578 for resurrecting this, @derickr and @Ocramius for reviewing this, and to @nikic for merging this. I also really appreciate the contribution credit received and noted in the NEWS file.

Honestly, I was quite disheartened with the PHP contribution process nearly 3 years ago when I had originally created the #1145 PR. It was my first attempt at a code contribution to the PHP source, and I made quite an effort to make the contribution as clean and clear as possible, which was certainly a challenge with my incredibly limited C experience. Ultimately, the hasty actions taken by @smalyshev (2d9399a) with incredibly little communication based on a single message on the internals mailing list, was a really frustrating and unnecessary experience, as is now apparent by the merging of this recreated work.

Ultimately, I'm happy to see the code being merged in, and I appreciate the contribution credit, however I wish to make a plea with the internals team to reflect on why this user-friendly and simple change was now considered and merged in nearly 3 years and 4 minor releases later. I only make this plea to hope that we can further encourage contribution in the future, rather than seemingly discouraging it.

Thank you! 🙂

Additional context:

@derickr
Copy link
Member

derickr commented May 13, 2018

This addition has not been documented yet: http://php.net/manual/en/class.datetime.php — please add documentation.

@ryanotella
Copy link

I agree that the absence of this method blocks consistent transition of legacy projects to using immutable dates. TBH, it's been blocking us for several years now - every attempt has been reverted.

However, the method really needed to be in PHP 5.5 and not 7.3+.

  1. Anyone supporting a legacy project is unlikely to upgrade to 7.3+ in order to benefit from the method, so it's hard to care.
  2. Anyone running something edgish, probably isn't interested in legacy transitions, so it's hard to care.

And most people are one or the other most of the time, so nobody cares. I'd suggest that virtually nobody will benefit from this method for a couple of years. That doesn't mean it shouldn't happen.

weirdan added a commit to weirdan/psalm that referenced this pull request Jan 16, 2019
muglug pushed a commit to vimeo/psalm that referenced this pull request Jan 17, 2019
@ondrejmirtes
Copy link
Contributor

Hi, it seems like this method is missing from the documentation: https://www.php.net/manual/en/class.datetime.php

@cmb69
Copy link
Member

cmb69 commented Mar 4, 2021

Hi, it seems like this method is missing from the documentation:

FTR, it has been documented in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.